Repository navigation
Conversation
 [Critical impact] The latest changes have no identified code blocker; the separately requested security-owner approval remains pending. SummaryAdds Fireblocks BYOK setup, encrypted credentials, named vault selection, and native SOL/USDC payments. Payments keep a durable request identity and use read-only checks to resolve uncertain outcomes.
Reviews (11) · Last reviewed commit: "fix(fireblocks): address route and schem..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
672be2c to
644893a
Compare
|
React Doctor found no new issues. 🎉 Reviewed by React Doctor for commit |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
keeganthomp
left a comment
keeganthomp
left a comment
There was a problem hiding this comment.
Requesting changes for fireblocks-transfer.ts:223 (ADR 0007 re-admission), :259 (429s wedge payments), :321 (swallowed settlement mismatch) and handlers.ts:953 (refusals return 200), plus the missing SDLC §7 tier, threat-model, dependency and AI-use lines; Greptile's eight items are still open.
| }); | ||
| if (!claimed) return currentRow(repo, row); | ||
| try { | ||
| await assertMoneyStartAdmitted( |
There was a problem hiding this comment.
ADR 0007 requires pending-approval signing to re-run decideMovement at execution. This admits once before the POST, so a key revoked or org suspended while Fireblocks awaits approval still pays out. Cancel open Fireblocks transactions on revocation, or amend ADR 0007 with security sign-off, which the value-movement allowlist entry needs too.
|
human translation: I propose we explicitly document and allow a race condition where someone can make a Fireblocks proposal to TRANSFER from within SDP, then, before the TRANSFER is approved in Fireblocks, you revoke the key that made the proposal in SDP... at this point, SDP doesn't do anything to attempt to prevent the transfer. I think that's okay and we should allow that behavior and document it. Detailed AI Slop for the Curious of Mind: Following up on the ADR 0007 re-admission concern: I propose retaining native Fireblocks The remaining behavior is:
We have documented this limitation and implemented safeguards around submission, tenant isolation, idempotency, and settlement verification. We have not eliminated the revocation race or implemented automatic cancellation on SDP revocation. The proposed boundary treats an accepted—or potentially accepted—Fireblocks request as an externally committed instruction. Revocation blocks new SDP submissions; incident response must separately address pending transfers in Fireblocks. Automatic cancellation could reduce exposure, but can race execution and would not guarantee compliance with the existing pre-sign authorization requirement. Requesting explicit, named security-owner approval for both the ADR 0007 exception and the This comment proposes a decision; it does not record security approval. Please keep the security thread open until that decision is made. If the exception is declined, we should disable native sends until an enforceable pre-sign authorization flow is available. |
Yeah I think this is a reasonable exception |
Windscar
left a comment
Windscar
left a comment
There was a problem hiding this comment.
Good work. The transfer recovery and settlement paths look sound, but credential rotation can leave imported vault wallets unusable.
Reviewed: Submission idempotency, reconciliation, provider responses, credential and database state transitions, tenant scope, release gates, dashboard flows, and relevant tests.
Required for approval:
- Verify every attached Fireblocks vault before credential rotation or rollback; details inline.
Non-blocking:
- Limit Fireblocks Pay choices to SOL and USDC; the wizard currently offers tokens the API rejects.
6405b37 to
f0d6845
Compare
Windscar
left a comment
Windscar
left a comment
There was a problem hiding this comment.
Good work. One recovery path still needs to handle credentials that become unavailable after Fireblocks accepts a transfer.
Reviewed: Prior review threads and fixes; BYOK credential lifecycle, payment admission and idempotency, settlement reconciliation, dashboard flows, and relevant tests.
Required for approval:
- Keep accepted Fireblocks transfers reconcilable after connection deactivation or a failed credential check; details inline.
b34da27 to
01502f7
Compare
Generate dedicated organization/project RSA credentials and a downloadable CSR. Guide API-user registration, discover named vaults, and import selected wallets with encrypted BYOK credentials and tenant isolation. Support credential rotation, rollback, deactivation and secret cleanup. Use native Fireblocks TRANSFER for direct SOL and USDC payments. Resolve USDC by exact mint/network, persist one submission attempt with a stable external transaction ID, and reconcile approval/broadcast status without automatic resubmission. Show provider status, substatus and fee responsibility in the dashboard. Other signing paths retain solana-keychain RAW signing. Validate custody, policy, idempotency, concurrent submission, uncertain outcomes, tenant boundaries, status tracking, dashboard and live-test runner behavior. API/web typechecks, module boundaries, migration compatibility and secret serialization checks pass. Verify the real Fireblocks asset catalogue and local mainnet UI without submitting a native payment. Deploy migrations 0127 and 0128 before the BYOK runtime. Native payments require TRANSFER and destination-policy permission plus SOL for fees in the source vault. RAW permission is only required for separate custom signing and the optional signer check.
Validate provider boundaries and preserve credential identity through setup and rotation. Return definitive payment refusals, log reconciliation failures, bound unacknowledged recovery, and keep durable provider deduplication. Remove unused proposal schema and privileged policy reads, repair setup pending state and React Doctor findings, and harden the opt-in live runner. Retain native TRANSFER with a proposed ADR 0007 exception that still requires named security approval; no claim of approval or atomic revocation is made.
Preserve selected vault readiness after failed pagination and explicitly bind credential rotation requests to their provider. Reuse shared input styling, centralize transfer status mapping, and remove unchecked assertions and nested UI branching. Add regression tests for pagination failures and rotation provider mismatches, and verify live-runner configuration and receipt data boundaries.
…tion Keep COMPLETED create responses and detailed responses without a transaction signature processing until a detailed read verifies the payment intent, destination and exact settled amount. Make settlement verification an explicit input to the shared status mapping and cover eventual recovery without another transaction POST. Make the CSR retry regression test control action completion so it cannot race the parent transition when the next registration form mounts.
…ment Verify each attached vault through solana-keychain before rotation or rollback, compare its address with the stored wallet, and recheck the locked wallet set before committing. Preserve the current credential on failure and keep transient provider failures retryable. Each distinct vault requires one address GET, including the reused primary result; no Solana RPC calls are added. Limit both Pay layouts to SOL and canonical network USDC for Fireblocks BYOK, using the same supported-mint predicate as the API. Clear unsupported selections when switching wallets and reject stale selections before review. Validation: 224 API tests; 3,586 dashboard tests; all 28 workspace typecheck tasks; 119 changed-file Biome checks; module boundaries, value movement, SecretRef serialization, and React Doctor with no new findings. No live Fireblocks calls.
Keep main's recurring-payment expiry migration at 0152 and move the unchanged Fireblocks credential-constraint migration to unused number 0154. The CSR table does not depend on these constraints; the validation migration remains later. Document how existing local PR databases can rename the migration-ledger entry without replaying already-applied constraint changes or replacing payment data.
…vation Separate submission health checks from intent-bound recovery access. Before reading the retained active credential, verify the original connection, provider account, vault and source address. New payments still require an active healthy connection, and revoked credentials remain unavailable. Cover dashboard and cron settlement after deactivation and failed checks, blocked new submissions, mismatched intents, revoked credentials, and no further provider reads after settlement. No extra RPC or provider requests: each admitted poll still issues one Fireblocks GET and never resubmits. Validation: 95 focused API tests, API typecheck, Biome, module boundaries, value-movement and SecretRef serialization checks.
Resolve fresh custody access against the durable intent before the provider POST, fail definitive local refusals, and recover status without requiring an operational wallet. Cover custody transitions and receipt-bound read-only recovery.
Keep pending native payments refreshing while allowing ramps to use their existing live poller. Extract the refresh effect so the rebased transaction component stays within React Doctor complexity limits. Ramp reads use one polling path instead of both the ramp poller and a redundant 15-second page refresh. Native payment polling remains one visible-page refresh every 15 seconds. Cover settlement, missing transfer details, ramp exclusion, hidden tabs, and unmount cleanup.
01502f7 to
25a32a8
Compare
This branch was successfully deployed
4 participants
Summary
Add beta-only Fireblocks BYOK so organizations can connect their own Fireblocks workspace and send SOL or USDC payments through SDP.
Connection and payments
TRANSFERrequests. Fireblocks handles approval, signing, network fees, and broadcasting.Minimal integration scope
Validation
Operational notes
See
docs/ops/fireblocks-byok.mdfor setup instructions and the detailed security review.Security / SDLC §7
Risk tier: Critical — credential handling and fund movement.
Proposed ADR 0007 exception: retain native Fireblocks TRANSFER
I propose retaining Fireblocks native
TRANSFERand obtaining explicit, named security-owner approval for the exception described below.SDP checks the initiating principal’s live authorization, wallet scope, organization/project eligibility, and custody connection before submitting a transfer. Fireblocks subsequently controls approval, signing, and broadcasting. Our current integration has no execution callback that rechecks SDP authorization immediately before Fireblocks signs.
Consequently, a transfer submitted while access was valid can still execute after the initiating SDP key is revoked, the member loses access, the organization is suspended, or the connection is deactivated. Revocation prevents new SDP submissions, but does not withdraw an instruction already accepted by Fireblocks. An ambiguous submission outcome must also be treated as potentially accepted.
This differs from ADR 0007’s requirement to recheck authorization when an asynchronous signing request executes.
Proposed security boundary
Treat an accepted or potentially accepted Fireblocks transfer as an externally committed instruction:
Automatic cancellation on SDP revocation is not currently implemented. Adding it would be a useful mitigation, but would not guarantee that revocation wins a race with signing.
Threat model and controls
Relevant threats include cross-tenant credential or vault substitution, credential disclosure, duplicate payments after ambiguous responses, incorrect settlement attribution, and execution after SDP authorization is revoked.
Controls include tenant-scoped encrypted credentials, human-only connection management, beta gating, live authorization before submission, Fireblocks approval policies, durable external transaction IDs, no automatic resubmission after uncertainty, exact settlement verification, and sanitized logging.
The revocation race remains an explicit residual risk requiring approval.
Dependency assessment and AI use
This change adds
zod@4.6.5to the custody package for provider-response validation and@solana/keychain-core@1.4.0/@solana/keychain-fireblocks@1.4.0as API test dependencies. These versions already exist in the workspace dependency graph; the dependency and lockfile changes remain part of the security review.Codex assisted implementation, testing, and review remediation. Automated checks and AI-assisted review do not constitute independent human review or security approval.
Approval required before merge
Status: proposed; not yet security-approved.
Requesting named security-owner approval of both:
fireblocks-native-transferallowlist entry inscripts/check-value-movement.mjs.Record the reviewer, approval date, approval-comment link, and explicitly accepted exposure in the ADR. Until that approval is recorded, the security review item remains open.
If this boundary is not acceptable, native sends should remain disabled until we implement a flow that can enforce fresh SDP authorization before signing.